drivers: eeprom: stm32: HAL return value handling #97279
Conversation
zephyrbot
requested review from
FRASTM,
GeorgeCGV,
djiatsaf-st,
erwango,
gautierg-st,
henrikbrixandersen,
marwaiehm-st and
mathieuchopstm
drivers/eeprom/eeprom_stm32.c
Outdated
| if (HAL_FLASHEx_DATAEEPROM_Unlock() != HAL_OK) { | ||
| return -EIO; | ||
| } |
There was a problem hiding this comment.
Nit: there should be consistency about whether driver uses
if (xxx() != HAL_OK) {
...fail
}or
ret = xxx();
if (ret != HAL_OK) {
...fail
}There was a problem hiding this comment.
I think when the return value is needed only to be tested straight, it's not really needed to be stored then tested, as here. Is this function, below, the return value is used not only with an if() instruction.
That said, I'm fine using the latter way (among the 2 you mentioned above).
drivers/eeprom/eeprom_stm32.c
Outdated
| if (ret != HAL_OK) { | ||
| LOG_ERR("failed to write to EEPROM (err %d)", ret); | ||
| HAL_FLASHEx_DATAEEPROM_Lock(); | ||
| k_mutex_unlock(&lock); |
There was a problem hiding this comment.
Seomthing's wrong in the 1st commit. EEPROM should be locked back. I'll fix.
drivers/eeprom/eeprom_stm32.c
Outdated
| if (HAL_FLASHEx_DATAEEPROM_Unlock() != HAL_OK) { | ||
| return -EIO; | ||
| } |
There was a problem hiding this comment.
I think when the return value is needed only to be tested straight, it's not really needed to be stored then tested, as here. Is this function, below, the return value is used not only with an if() instruction.
That said, I'm fine using the latter way (among the 2 you mentioned above).
etienne-lms
force-pushed
the
stm32-hal-ret-eeprom
branch
from
bfde38d to
bc8ef62
Compare
drivers/eeprom/eeprom_stm32.c
Outdated
| ret = HAL_FLASHEx_DATAEEPROM_Lock(); | ||
| if (ret != HAL_OK) { | ||
| LOG_ERR("failed to lock to EEPROM (err %d)", ret); | ||
| } |
There was a problem hiding this comment.
HAL_FLASHEx_DATAEEPROM_Lock only implemented on L0/L1 and does not perform error checking.
I'm fine to perform the error checking, but LOG_ERR with value report seems overkill.
Correct eeprom_stm32_write() to return a valid errno instead of mixing HAL return values and errno return values. Clarify HAL return value is of type HAL_StatusTypeDef and not an int in eeprom_stm32_read(). Remove printing of HAL_FLASHEx_DATAEEPROM_Lock() error code since not very useful. Signed-off-by: Etienne Carriere <[email protected]>
Add missing test of HAL_FLASHEx_DATAEEPROM_Unlock() return value. By the way, add a error trace message when failing to relock the EEPROM after we failed to program it. Signed-off-by: Etienne Carriere <[email protected]>
etienne-lms
force-pushed
the
stm32-hal-ret-eeprom
branch
from
bc8ef62 to
430dc60
Compare
|
Review comment addressed. |
| HAL_FLASHEx_DATAEEPROM_Unlock(); | ||
| hal_ret = HAL_FLASHEx_DATAEEPROM_Unlock(); | ||
| if (hal_ret != HAL_OK) { | ||
| LOG_ERR("failed to unlock to EEPROM"); |
There was a problem hiding this comment.
LOG_ERR still feels overkill but ok
|
4 participants
Correct
eeprom_stm32_write()to return a valid errno instead of mixing HAL return values and errno return values.Clarify HAL return value is of type
HAL_StatusTypeDefand not anintineeprom_stm32_read().